Conversation
walterxie
commented
Jun 22, 2026
- fix resume bug for testGTR.xml;
- prevent silent fail if XML does not exist during the tests;
- upgrade tests using old framework junit 4 to junit 5;
- test more scenario for resume and add more XMLs to test resume.
- move all beastfx resume and XMLParsing tests (test/beastfx/integration/) to base, because they do not depend on beastfx now.
There was a problem hiding this comment.
-
ParameterUtils.java:53
The ID-strip fallback branch is effectively dead and throws instead of recovering. The new noboundPattern dropped the old leading^.*?wildcard and relies on fullStr.startsWith(id) having stripped the ID first. When startsWith(id) is false (e.g. getTextContent() returns " kappa: 29" with leading whitespace from a pretty-printed/hand-edited/foreign state file), str keeps the full text, and the anchored ^(?:{…})?: pattern matches neither. So parseParameter throws "String could not be parsed". The old ^.*? tolerated this. For files written by BEAST3's own paramToString (no leading whitespace) it round-trips fine, so this is a robustness regression on non-canonical input, not a break of the happy path. A fullStr.trim() (or a defined error rather than the silent fallback) would keep the robustness. -
XMLProducerTest.java:35
resolveNexusDir()duplicatesXMLPathUtil.resolveExamplesDir(). It re-implements the same getResource → toURI → File + user.dir fallback logic, differing only by appending /nexus. Since XMLProducerTest is in the same package, this could benew File(XMLPathUtil.resolveExamplesDir(), "nexus").getAbsolutePath(). Any future fix to the resolution strategy (jar URLs, fallback path) must now be made in two places. -
ResumeTest.java / XMLPathUtil.java:64
Output isolation rests on the JVM-wide file.name.prefix, making the test order-/fork-sensitive. setUpOutputDir(baseName) mutates a global system property, and ResumeTest reads it back post-run to locate the state and .log files, with an assertTrue(logFiles.length == 1) check. Under Surefire parallel or forkCount execution, ExampleXmlParsingTest/ExampleJSONParsingTest (which set the prefix to test/) can clobber it mid-test, producing flaky "expected exactly 1 .log file" / "state file missing" failures. Your javadoc acknowledges the limitation. It is worth either documenting the required /sequential constraint in the pom or flagging it explicitly. -
XMLPathUtil.java:8 (and ResumeTest reuse) have stale javadoc. The class doc says "Shared test infrastructure for beast-fx integration tests", but this PR moves it into beast-base (test.beast.integration) and all its callers are now in beast-base. The doc misdirects a future maintainer to the wrong module.
-
ResumeTest.java:62 A non-MCMC runnable yields a misleading failure. If any listed XML's resolves to a non-MCMC Runnable, the
if (runable instanceof MCMC mcmc)block is skipped, nothing runs, and execution falls through toassertTrue(sf.exists(), "…state file missing"). This will report "state file missing" rather than "wasn't an MCMC". All five current XMLs are MCMC, so this is latent, not active. -
ResumeTest.java:57-70 vs 94-107 The initial-run and resume blocks are near-duplicates.
They differ only in Logger.FILE_MODE(overwrite/resume) and the setStateFile resume flag; aparseAndRun(fileName, stateFile, mode, resume)helper would remove ~13 duplicated lines and the sync hazard.
…nOutputDirs to ResumeTest #106
|
I have resolved all the issues listed above; each commit generally corresponds to an issue in chronological order. |